chore(dao): Condense delete/bulk-delete operations #24466

john-bodley · 2023-06-20T21:26:49Z

SUMMARY

Per SIP-92 Proposal for restructuring the Python code base #24331 organized all the DAOs to be housed within a shared folder—the result of which highlighted numerous inconsistencies, repetition, and inefficiencies.

This PR (one of many smaller bitesized PRs) condenses the single (delete) and bulk deletion (bulk_delete) logic into a single (delete) method which removes a bunch of copypasta and helps to standardize the code.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue: [SIP-92] Proposal for restructuring the Python code base #20630
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

john-bodley · 2023-06-20T21:27:27Z

superset/daos/annotation.py

@@ -30,19 +27,6 @@
 class AnnotationDAO(BaseDAO):
    model_cls = Annotation

-    @staticmethod


Copypasta except the BaseDAO is defined as a classmethod as opposed to a staticmethod.

These comments are very helpful while reviewing. Thank you.

john-bodley · 2023-06-20T21:27:57Z

superset/daos/annotation.py

@@ -67,21 +51,6 @@ def validate_update_uniqueness(
 class AnnotationLayerDAO(BaseDAO):
    model_cls = AnnotationLayer

-    @staticmethod
-    def bulk_delete(


Copypasta except the BaseDAO is defined as a classmethod as opposed to a staticmethod.

Nice cleanup!

john-bodley · 2023-06-20T21:29:21Z

superset/daos/css.py

 from superset.models.core import CssTemplate

 logger = logging.getLogger(__name__)


 class CssTemplateDAO(BaseDAO):
    model_cls = CssTemplate
-
-    @staticmethod


Copypasta except the BaseDAO is defined as a classmethod as opposed to a staticmethod.

codecov · 2023-06-20T21:37:17Z

Codecov Report

Merging #24466 (f1fae05) into master (aa6870b) will decrease coverage by 10.75%.
The diff coverage is 47.80%.

❗ Current head f1fae05 differs from pull request most recent head 8d32885. Consider uploading reports for the commit 8d32885 to get more accurate results

@@             Coverage Diff             @@
##           master   #24466       +/-   ##
===========================================
- Coverage   69.08%   58.33%   -10.75%     
===========================================
  Files        1906     1906               
  Lines       74156    74107       -49     
  Branches     8161     8165        +4     
===========================================
- Hits        51232    43234     -7998     
- Misses      20805    28750     +7945     
- Partials     2119     2123        +4

Flag	Coverage Δ
hive	`54.14% <43.38%> (+0.20%)`	⬆️
mysql	`?`
postgres	`?`
presto	`54.04% <43.38%> (+0.20%)`	⬆️
python	`61.10% <52.20%> (-22.38%)`	⬇️
sqlite	`?`
unit	`54.79% <52.20%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...et-ui-chart-controls/src/components/labelUtils.tsx	`100.00% <ø> (ø)`
superset-frontend/src/components/Tooltip/index.tsx	`100.00% <ø> (ø)`
...ses/DatabaseModal/DatabaseConnectionForm/index.tsx	`81.81% <ø> (ø)`
...otation_layers/annotations/commands/bulk_delete.py	`50.00% <0.00%> (-37.50%)`	⬇️
superset/annotation_layers/commands/bulk_delete.py	`46.15% <0.00%> (-38.47%)`	⬇️
superset/charts/commands/bulk_delete.py	`44.44% <0.00%> (-46.99%)`	⬇️
superset/css_templates/commands/bulk_delete.py	`50.00% <0.00%> (-37.50%)`	⬇️
superset/daos/annotation.py	`56.00% <ø> (-31.24%)`	⬇️
superset/dashboards/api.py	`51.28% <0.00%> (-41.29%)`	⬇️
superset/dashboards/commands/bulk_delete.py	`44.44% <0.00%> (-47.23%)`	⬇️
... and 37 more

... and 267 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

john-bodley · 2023-06-21T05:33:07Z

superset/annotation_layers/annotations/commands/delete.py

        try:
-            annotation = AnnotationDAO.delete(self._model)


No need to return the object we're deleting.

john-bodley · 2023-06-21T16:28:32Z

superset/dashboards/api.py

@@ -1364,7 +1364,7 @@ def delete_embedded(self, dashboard: Dashboard) -> Response:
              $ref: '#/components/responses/500'
        """
        for embedded in dashboard.embedded:
-            DashboardDAO.delete(embedded)
+            EmbeddedDashboardDAO.delete(embedded)


I'm somewhat surprised that #24465 didn't pick this up.

john-bodley · 2023-06-21T17:27:38Z

superset/annotation_layers/annotations/commands/bulk_delete.py

@@ -36,8 +36,10 @@ def __init__(self, model_ids: list[int]):

    def run(self) -> None:
        self.validate()
+        assert self._models


This logic is temporary. It should be cleaned up when we refactor the commands and enforce that validate doesn't augment any properties.

john-bodley · 2023-06-21T17:28:50Z

superset/daos/dataset.py

        """
        Deletes a Dataset column
        """
-        return cls.delete(model, commit=commit)
+        return DatasetColumnDAO.delete(model, commit=commit)


This is more accurate, i.e., the DatasetColumnDAO rather than the DatasetDAO should be deleting the column.

john-bodley · 2023-06-21T17:30:21Z

superset/datasets/metrics/commands/delete.py

        try:
-            if not self._model:


DRY. The validate method already raises a DatasetMetricNotFoundError if the metric is not found.

john-bodley · 2023-06-21T17:31:18Z

superset/daos/base.py

    @classmethod
-    def delete(cls, model: T, commit: bool = True) -> T:
+    def delete(cls, objects: T | list[T], commit: bool = True) -> None:


items seems to be a more accurate/consistent than models, i.e., we're deleting database items/instances as opposed to models.

john-bodley · 2023-06-22T04:22:20Z

superset/daos/dataset.py

-    def bulk_delete(models: Optional[list[SqlaTable]], commit: bool = True) -> None:
-        item_ids = [model.id for model in models] if models else []
+    @classmethod
+    def delete(


The bulk_deletion logic differs from the delete logic in the sense that the former first deletes the relationships (which in actuality may not be explicitly required) before deleting the dataset, whereas the later only deletes the datasets (which in theory should delete all the records with an explicit relationship).

michael-s-molina

LGTM

michael-s-molina · 2023-07-06T12:26:37Z

superset/daos/annotation.py

@@ -30,19 +27,6 @@
 class AnnotationDAO(BaseDAO):
    model_cls = Annotation

-    @staticmethod


These comments are very helpful while reviewing. Thank you.

michael-s-molina · 2023-07-06T12:26:58Z

superset/daos/annotation.py

@@ -67,21 +51,6 @@ def validate_update_uniqueness(
 class AnnotationLayerDAO(BaseDAO):
    model_cls = AnnotationLayer

-    @staticmethod
-    def bulk_delete(


Nice cleanup!

superset/daos/base.py

michael-s-molina · 2023-07-06T12:51:53Z

Thank you for the nice cleanup @john-bodley!

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> (cherry picked from commit 7409289)

pull-request-size bot added the size/L label Jun 20, 2023

john-bodley commented Jun 20, 2023

View reviewed changes

john-bodley requested review from michael-s-molina, dpgaspar and villebro June 20, 2023 21:30

john-bodley force-pushed the john-bodley--dao-bulk-delete branch 4 times, most recently from 2208e64 to 51dc939 Compare June 21, 2023 05:32

john-bodley commented Jun 21, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--dao-bulk-delete branch 5 times, most recently from 5b99d4b to 8b63521 Compare June 21, 2023 16:24

john-bodley commented Jun 21, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--dao-bulk-delete branch 2 times, most recently from 02e8b79 to cbb621d Compare June 21, 2023 17:26

john-bodley commented Jun 21, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--dao-bulk-delete branch 7 times, most recently from ee29647 to 3620631 Compare June 22, 2023 03:58

john-bodley commented Jun 22, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--dao-bulk-delete branch from 3620631 to aa543ae Compare June 22, 2023 04:54

john-bodley changed the title ~~chore(dao): Condense delete/bulk-delete~~ chore(dao): Condense delete/bulk-delete operations Jun 22, 2023

john-bodley mentioned this pull request Jun 23, 2023

chore(dao): Add explicit ON DELETE CASCADE when deleting datasets #24488

Merged

9 tasks

john-bodley requested a review from betodealmeida June 23, 2023 14:16

john-bodley force-pushed the john-bodley--dao-bulk-delete branch from aa543ae to 14664d8 Compare June 28, 2023 23:29

chore(dao): Condense delete/bulk-delete

6aaeed0

john-bodley force-pushed the john-bodley--dao-bulk-delete branch from 14664d8 to 6aaeed0 Compare June 30, 2023 03:26

michael-s-molina approved these changes Jul 6, 2023

View reviewed changes

Update superset/daos/base.py

8d32885

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

john-bodley merged commit 7409289 into apache:master Jul 6, 2023

john-bodley mentioned this pull request Jul 6, 2023

chore(command): Condense delete/bulk-delete operations #24607

Merged

9 tasks

michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 12, 2023

michael-s-molina pushed a commit that referenced this pull request Jul 26, 2023

chore(dao): Condense delete/bulk-delete operations (#24466)

71eff95

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> (cherry picked from commit 7409289)

john-bodley mentioned this pull request Nov 1, 2023

[SIP-99D] Proposal for consolidating Command/DAO create, update, and upsert operations #25830

Closed

mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(dao): Condense delete/bulk-delete operations #24466

chore(dao): Condense delete/bulk-delete operations #24466

john-bodley commented Jun 20, 2023 •

edited

Loading

john-bodley Jun 20, 2023

michael-s-molina Jul 6, 2023

john-bodley Jun 20, 2023

michael-s-molina Jul 6, 2023

john-bodley Jun 20, 2023

codecov bot commented Jun 20, 2023 •

edited

Loading

john-bodley Jun 21, 2023

john-bodley Jun 21, 2023

john-bodley Jun 21, 2023

john-bodley Jun 21, 2023

john-bodley Jun 21, 2023

john-bodley Jun 21, 2023 •

edited

Loading

john-bodley Jun 22, 2023

michael-s-molina left a comment

michael-s-molina Jul 6, 2023

michael-s-molina Jul 6, 2023

michael-s-molina commented Jul 6, 2023

chore(dao): Condense delete/bulk-delete operations #24466

chore(dao): Condense delete/bulk-delete operations #24466

Conversation

john-bodley commented Jun 20, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 20, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina commented Jul 6, 2023

john-bodley commented Jun 20, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading

john-bodley Jun 21, 2023 •

edited

Loading